Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft: Format .cabal files with cabal-fmt #4230

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

fendor
Copy link
Collaborator

@fendor fendor commented May 13, 2024

Compare with #4229


library
import: common-deps
-- configuration
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, so this is what I remembered and is IMO a deal-breaker for cabal-fmt: it doesn't preserve comments properly. cabal-gild seems better (but not perfect), so that's a strong preference for me.

@fendor fendor force-pushed the enhance/format-with-cabal-fmt branch from 9e7071e to a8708fc Compare May 13, 2024 09:51
Copy link
Collaborator

@jhrcek jhrcek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

C


ghc-options: -Werror -Wwarn=unused-packages

-- Note [unused-packages]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment moved from where it belongs.


-- FIXME: Only needed to workaround for qualified imports in GHC 9.4
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment moved

autogen-modules: Paths_haskell_language_server
ghc-options: -threaded -rtsopts "-with-rtsopts=-I0 -A128M"

-- allow user RTS overrides
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comments moved


if flag(isolateCabalfmtTests)
if flag(isolatecabalfmttests)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TIL Cabal converts flag names to lowercase.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is weird, is that cabal-fmt preference? It seems like cabal-gild doesn't convert to lower case.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gild currently doesn't convert flags to lowercase, but it probably will after I fix tfausak/cabal-gild#72. Conditionals in Cabal are represented by Condition ConfVar, which can have a FlagName in them. I can't currently find any documentation about it, but flag names are explicitly converted to lowercase when parsing: https://github.com/haskell/cabal/blob/03d98294e1c071be368f04dbe6fc9cb263bea86d/Cabal-syntax/src/Distribution/Types/Flag.hs#L117

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's some discussion here (and in linked issues): haskell/cabal#5043

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW I think that just because cabal compares flag names case-insensitively, that's not necessarily a reason to normalize the user's flag name to lower-case. And it would be fairly unwelcome...

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Starting with version 1.3.1.3, Gild now behaves the same as cabal-fmt in that it converts flag names into lowercase. This is a consequence of going through the FlagName type from Cabal-syntax.

@fendor fendor marked this pull request as draft June 13, 2024 21:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants